fix(auth): enable native keyring backends on top of #359#373
fix(auth): enable native keyring backends on top of #359#373zssz wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: fb45eaa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical authentication persistence issue where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical authentication persistence bug by enabling native keyring backends and refactoring credential loading. The changes enhance security by utilizing strong cryptographic primitives and secure file permissions, with no high or critical security vulnerabilities identified. My feedback focuses on improving the robustness of the new test to prevent potential issues in CI environments.
|
/gemini review |
|
Thanks for this PR! #359 has been merged which adds Could you rebase this on top of the latest main (which includes #359) so we can evaluate the two together? The credential_store.rs has changed significantly. |
zssz
changed the title
I rebased the branch onto Updated PR title and description aswell. |
zssz
force-pushed
the
zssz/fix-auth-keyring-backend
branch
from
32b0177 to
b0e72c5
Compare
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical issue where authentication credentials were not being persisted securely due to the keyring crate silently falling back to an in-memory mock backend. By enabling the native keyring features in Cargo.toml, the application will now correctly use the host OS's secure storage, with a robust file-based fallback. The additional fix to serialize a test that was subject to a race condition is also a good catch and correctly implemented. The changes are well-executed and significantly improve the security and reliability of credential management.
|
D-Bus dependency: This introduces a build-time dependency on libdbus-1-dev for Linux (which is why CI is failing). Many of our users run
|
|
/gemini review |
Implemented in I changed
That keeps I also regenerated |
There was a problem hiding this comment.
Code Review
This pull request correctly enables native keyring backends for macOS and Windows by using target-specific dependencies in Cargo.toml. This change addresses a critical issue where credentials could be silently stored in an in-memory mock backend instead of the secure OS store. The implementation is sound and follows best practices for platform-specific features in Rust. Additionally, the pull request includes a fix for a test race condition in src/auth_commands.rs by correctly marking a test that interacts with environment variables as serial. The changes are well-explained and improve the security and reliability of credential storage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
==========================================
+ Coverage 62.17% 64.39% +2.21%
==========================================
Files 38 38
Lines 14797 15583 +786
==========================================
+ Hits 9200 10034 +834
+ Misses 5597 5549 -48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Description
This PR is now rebased on top of the latest
main, including #359.Motivation:
#359 added
GOOGLE_WORKSPACE_CLI_KEYRING_BACKENDand preserved.encryption_keyas a durable fallback, but it does not change how thekeyringcrate itself is compiled. The remaining problem is thatkeyringwas still declared without any native backend features, sokeyringv3 could silently fall back to its in-memory mock backend. In practice, that meantgws auth logincould appear to succeed, while the selectedkeyringbackend was not actually backed by the host OS secure store.Why this fixes it:
keyringbackends inCargo.toml, so the defaultkeyringbackend is backed by a real platform implementation instead of the in-memory mock store.gwsboth real OS-keyring support when available and a persistent.encryption_keyfallback when it is not.credential_store.rsaligned withmainfrom feat(credential_store): add GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND env var #359 rather than reintroducing overlapping logic.Additional local fix from verification:
While rebasing and verifying on top of #359,
cargo testexposed a test race inauth_commands::tests::config_dir_returns_gws_subdir, where it could observeGOOGLE_WORKSPACE_CLI_CONFIG_DIRwhile another serialized test was mutating it. This PR marks that test as#[serial_test::serial]as well.Note on
Cargo.lock:The
Cargo.lockdiff is large because enabling the nativekeyringbackends pulls in the transitive platform-specific dependencies needed for persistent secure storage.This is not adding a new command or Discovery request path, so there is no relevant
--dry-runoutput for the template section below.Dry Run Output:
{ "not_applicable": "This change updates auth credential backend compilation and a test isolation fix; it does not add a new command or API request body." }Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.